✨ PHP #61
✨ PHP #61
Conversation
WalkthroughThe pull request introduces a new PHP language package ( Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (8)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/php/nursery.js (1)
12-17: Consider adding more comprehensive test casesWhile the current test is functional, it's using a very simple PHP string ('123'). Consider adding more complex PHP code examples that test various language features to ensure comprehensive parser coverage.
For example:
testRunner: parse => { - const sg = parse('123') - const root = sg.root() - const node = root.find('123') - assert.equal(node.kind(), 'text') + // Test simple value + const simpleTest = parse('123') + const simpleNode = simpleTest.root().find('123') + assert.equal(simpleNode.kind(), 'text') + + // Test PHP syntax + const phpTest = parse('<?php $var = "test"; echo $var; ?>') + const phpRoot = phpTest.root() + assert(phpRoot.findAll('echo').length > 0, 'Should parse echo statement') + assert(phpRoot.findAll('variable_name').length > 0, 'Should parse variable names') },packages/php/README.md (1)
16-24: Enhance usage example with realistic PHP codeThe usage example is clear, but could be improved by showing a more realistic PHP code snippet and demonstrating more operations on the parsed result.
Consider enhancing the example like this:
```js import php from '@ast-grep/lang-php' import { registerDynamicLanguage, parse } from '@ast-grep/napi' registerDynamicLanguage({ php }) -const sg = parse('php', `your code`) -sg.root().kind() +const sg = parse('php', `<?php + $greeting = "Hello, World!"; + echo $greeting; +?>`) + +// Get the root node and its kind +const root = sg.root(); +console.log("Root node kind:", root.kind()); + +// Find all variable declarations +const variables = root.findAll('variable_name'); +console.log("Found", variables.length, "variables"); + +// Find all echo statements +const echoStatements = root.findAll('echo'); +console.log("Found", echoStatements.length, "echo statements");packages/php/index.js (1)
1-9: Consider adding more PHP file extensions for comprehensive coverage.The implementation looks correct and well-structured. However, PHP also supports other file extensions like
.phtml,.php4,.php5,.php7, and.phps. For more comprehensive language support, consider adding these to the extensions array.- extensions: ['php'], + extensions: ['php', 'phtml', 'php4', 'php5', 'php7', 'phps'],scripts/nursery/index.ts (1)
51-52: Consider using nullish coalescing operator.The implementation works correctly, but using the nullish coalescing operator (
??) would be more semantically appropriate than logical OR (||) for setting default values when handling undefined/null parameters.- const src = config.src || 'src' + const src = config.src ?? 'src'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/php/README.md(1 hunks)packages/php/index.d.ts(1 hunks)packages/php/index.js(1 hunks)packages/php/nursery.js(1 hunks)packages/php/package.json(1 hunks)packages/php/postinstall.js(1 hunks)scripts/nursery/index.ts(2 hunks)
🔇 Additional comments (7)
packages/php/postinstall.js (1)
1-4:❓ Verification inconclusive
Consider adding the
srcparameter to match PR objectivesAccording to the PR objectives, a key change is adding a
srcparameter toSetupConfigto handle the non-standard location of source files in thetree-sitter-phppackage. However, this parameter isn't being passed to thepostinstallfunction here.Consider adding the
srcparameter to specify the correct source directory path (php/srcorphp_only/src).
🏁 Script executed:
#!/bin/bash # Checking if the postinstall function from @ast-grep/setup-lang accepts a src parameter grep -r "postinstall.*src" --include="*.js" .Length of output: 47
Post-install configuration: Add the
srcparameter fortree-sitter-phpThe current code in
packages/php/postinstall.jsonly passes thedirnameproperty to thepostinstallfunction. Based on the PR objectives—where handling a non-standard source directory is a key objective—it appears that asrcparameter should also be provided to correctly point to the location of the source files (e.g.,php/srcor an appropriate alternative).
- Action Required:
- Confirm that the
postinstallfunction from@ast-grep/setup-langaccepts asrcparameter.- Update the call in
packages/php/postinstall.jsto include the appropriatesrcproperty. For example:const { postinstall } = require('@ast-grep/setup-lang') postinstall({ dirname: __dirname, src: 'php/src' // Adjust this path if the correct directory differs })- Next Steps:
Please review the documentation or implementation of@ast-grep/setup-langto verify that this change is necessary and adjust the source path as needed.packages/php/index.d.ts (1)
1-10: Type definition looks goodThe
LanguageRegistrationtype is well-defined with the appropriate required and optional properties needed for language registration. The default export matches the usage pattern demonstrated in the README.packages/php/nursery.js (1)
6-18: Implementation correctly addresses the source path issueThe
srcparameter is set topath.join('php', 'src'), which properly addresses the non-standard location of source files in thetree-sitter-phppackage as mentioned in the PR objectives.packages/php/README.md (1)
5-12: Installation instructions are clear and completeThe installation instructions correctly specify all required dependencies and note the potential need for the tree-sitter CLI if prebuilt binaries aren't available.
scripts/nursery/index.ts (1)
25-28: Good documentation of the new parameter.The JSDoc comments clearly explain the purpose of the new parameter and provide helpful examples of packages that might need this functionality.
packages/php/package.json (2)
7-7: Note about the build script configuration.The build script correctly points to the PHP directory in tree-sitter-php package, addressing the main issue described in the PR objectives. This approach directly uses the source from node_modules rather than using the copied src directory, which appears to be intentional as mentioned in your PR description.
13-20: Files configuration looks complete.The files array includes all necessary components for proper package functionality, including the main JavaScript file, type definitions, installation scripts, and prebuilds.
NatoBoram
force-pushed
the
feature/php
branch
2 times, most recently
from
20f96a4 to
53a9216
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/php/package.json (2)
1-5: Metadata Fields Need CompletionThe basic metadata is defined correctly. However, the
"description"field is currently empty and the"author"field is blank. Consider adding a brief package description and proper author information to improve discoverability and documentation on npm.
21-23: Optional: Enhance Package DescriptionWhile the
"keywords"and"license"fields are provided, the lack of an"author"and a non-existent"description"may make the package less informative for users. Consider providing these details to clarify the package’s purpose and origin.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
.changeset/wet-nights-cover.md(1 hunks)packages/php/README.md(1 hunks)packages/php/index.d.ts(1 hunks)packages/php/index.js(1 hunks)packages/php/nursery.js(1 hunks)packages/php/package.json(1 hunks)packages/php/postinstall.js(1 hunks)scripts/nursery/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/wet-nights-cover.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/php/nursery.js
- packages/php/README.md
- packages/php/postinstall.js
- packages/php/index.d.ts
- scripts/nursery/index.ts
- packages/php/index.js
🔇 Additional comments (5)
packages/php/package.json (5)
6-12: Review Build & Script CommandsThe scripts section is generally well-structured. In particular, note the build command:
"build": "tree-sitter build -o parser.so ./node_modules/tree-sitter-php/php"According to the PR objectives, there is some uncertainty about the usage of a separately copied
srcfolder versus directly referencing thenode_modules/tree-sitter-php/phpdirectory. Please confirm that this hard-coded path is intentional. If the newsrcfolder is meant to be used, the build command might need adjustment.
13-20: Files Array Validation and InclusionThe
"files"array includes"src"and"prebuilds", which is appropriate for packaging. Please verify that including the"src"folder is necessary and does not conflict with the build command’s reference to the PHP directory directly undernode_modules.
24-34: Dependencies and Peer DependenciesThe dependency configuration appears solid. The runtime dependency on
@ast-grep/setup-langand the peer dependency ontree-sitter-cli(along with its optional metadata) follow best practices. Ensure that version"0.24.6"fortree-sitter-cliand its inclusion in both peerDependencies and devDependencies are fully verified with your build and deployment environments.
35-39: Development Dependencies CheckThe devDependencies list includes
@ast-grep/nursery,tree-sitter-cli, andtree-sitter-php. Make sure that theworkspace:*reference for@ast-grep/nurseryresolves correctly in your CI and that the pinned versions fortree-sitter-cliandtree-sitter-phpare compatible with the rest of your tooling.
40-46: Publish and PNPM Configuration ValidationThe
"publishConfig"and"pnpm"sections are well defined. The publishConfig ensures the package is publicly accessible, and the PNPM configuration restricts published content to only the built dependencies. Just verify that these settings align with your deployment workflow.
NatoBoram
force-pushed
the
feature/php
branch
4 times, most recently
from
9c914c8 to
4a3ec25
Compare
2 participants
This is a weirder one.
The
tree-sitter-phppackage has itssrcfolder inphpandphp_onlyinstead of at its root.copySrcIfNeeded, however, is hard-coded to copytree-sitter-*/src, which doesn't exist.langs/scripts/nursery/index.ts
Lines 44 to 54 in 002639c
This PR adds a
srcparameter toSetupConfigto enable the nursery to find the appropriatesrcfolder in case it deviates fromsrc.https://github.com/coderabbitai/langs/blob/e8e4b4a097a1650711922e66ec60d8d84cde0b81/scripts/nursery/index.ts#L48-L60
The next issue is that the usual
"build": "tree-sitter build -o parser.so"script expects thesrcto not reference anything outside of itself, which is an issue forscanner.c.https://github.com/tree-sitter/tree-sitter-php/blob/576a56fa7f8b68c91524cdd211eb2ffc43e7bb11/php/src/scanner.c#L1
I changed it to
tree-sitter build -o parser.so ./node_modules/tree-sitter-php/php, but then it's not using thesrcfolder we just copied and then I have no idea why there's even asrcfolder in the first place if we can directly usenode_modules/tree-sitter-php/php/srcwithout issues. I notice thatsrcis published, but beyond that, I don't know how it's actually used.This issue affects these languages:
Since I don't know if this is a good implementation, I'll wait until this one is resolved / merged before adding support for those languages.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests